Repository navigation
Fix[mqbs]: release proxy message refs only for pushed Apps - #1810
Draft
chrisbeard wants to merge 8 commits into
Draft
chrisbeard wants to merge 8 commits into
chrisbeard wants to merge 8 commits into
Conversation
On a proxy, a message's refCount counts only the Apps it was pushed to, but every App slot of a new DataStreamMessage starts as e_PUT, which counts as pending. Removing the virtual storage of an App that never received the message still releases a reference, so a later CONFIRM from one App reaches zero references while another App holds the message. This test reproduces that bug in the storage layer with minimal step. Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
Contributor
Author
|
Test failure, as expected: TEST /home/runner/work/blazingmq/blazingmq/src/groups/mqb/mqbs/mqbs_inmemorystorage.t.cpp CASE 22 |
On a proxy, a message's refCount includes only the Apps named in its PUSH. However, every App slot starts in e_PUT, which appears pending. When an App loses its last reader, the proxy removes its virtual storage. This wrongly releases references for messages that were never pushed to that App. The refCount then falls below the number of Apps that still hold the message. A subsequent CONFIRM can consequently delete the message while another App still needs it. If that App's copy is in the PushStream, the proxy aborts when it tries to load the deleted message. If the copy is in a redelivery list, the message is silently dropped. Add integration coverage for three variants: - An App closes while another App has a PushStream backlog. - An App crashes while another App has both PushStream and redelivery backlogs. - An App crashes while another App has only a redelivery backlog. The tests verify that the proxy remains alive and that every remaining App receives all its messages. Note that three apps are required to trigger the bug, the proxy does not assert for cases with only two Apps Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
On a proxy, a message's refCount equals the number of Apps that received that message in a PUSH. The proxy marks exactly those Apps as 'e_PUSH'. Every other App slot keeps the initial 'e_PUT', which counts as pending. When an App lost its last reader, 'purgeImpl' released a reference for every pending slot, so messages never pushed to that App lost a reference. A later CONFIRM from one App then removed the message while another App still had it queued, leading to silent message loss or an assert in 'PushStreamIterator'. Add 'VirtualStorageCatalog::holdsReference' and release references only for App slots that hold one. Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
dorjesinpo
previously approved these changes
Oct 14, 2026
| const bool holdsRef = holdsReference( | ||
| appMessageView(*dataStreamMessage, vs->ordinal())); | ||
|
|
||
| if (vs->remove(dataStreamMessage, replacingOrdinal) && holdsRef) { |
Contributor
There was a problem hiding this comment.
It is important that remove gets called always.
Contributor
|
|
'remove' must run for every message, even when this App holds no reference, so explicitly call it on its own instead of inside of an if statement. After looking more closely though, the check can be simplified to just use 'holdsRef'; checking the return value of 'remove' is redundant. Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
… other Apps' messages After a proxy restarts, a consumer can confirm messages it was pushed before the restart. If the restarted proxy was pushed those messages for other Apps only, the CONFIRM from the unpushed App still releases a reference, so a later CONFIRM from one of those Apps deletes a message another App still has queued. This can cause a consumer to incorrectly never receive the the message. test_confirm_after_proxy_restart reproduces this bug using three Apps. This test fails until the fix in a following commit. Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
On a proxy, a message's refCount counts only the Apps it was pushed to, but an App it was never pushed to keeps the initial 'e_PUT' slot, which counts as pending. A CONFIRM from that App still released a reference, so a later CONFIRM from another App could reach zero references and delete the message while a third App still held it. This test reproduces the bug in the storage layer; it fails until the following commit. Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
On a proxy, a message's refCount is the number of Apps it was pushed to.
Those Apps are marked 'e_PUSH'; every other App slot keeps the initial
'e_PUT', which still counts as pending. 'VirtualStorageCatalog::confirm'
released a reference for any pending slot, so a CONFIRM from an App the
message was never pushed to still decremented the refCount. The refCount
could then reach zero, removing the message while another App still had
it queued, leading to silent message loss or an assert in
'PushStreamIterator'.
Check 'holdsReference' before confirming: when the App holds no
reference, return 'e_NON_ZERO_REFERENCES' and leave the refCount
unchanged.
This also corrects two unit tests whose proxy setup left counted Apps as
'e_PUT', which a real proxy does not do ('storePushIfProxy' marks every
counted App as 'e_PUSH').
Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
The new refCount unti tests used a local 'PooledBlobBufferFactory' declared after the 'Tester' which then violated sanitizers upon destruction. Add the message with 'Tester::addMessages' instead, as the other tests do. Its blobs come from the mock cluster's buffer factory, which outlives the storage, and its 'numPushApps' argument replaces the hand-written 'put' and 'setPushState' calls. Signed-off-by: Christopher Beard <cbeard9@bloomberg.net>
chrisbeard
changed the title
chrisbeard
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix proxy message reference accounting when an App did not receive a message.
A proxy counts references only for Apps included in a PUSH. However, every App slot initially has the pending
e_PUTstate, while only pushed Apps transition toe_PUSH. Two paths incorrectly treated any pending slot as holding a reference:Either path could reduce the reference count below the number of Apps still holding the message. A later CONFIRM could then delete the message while another App still had it queued, resulting in silent message loss from a redelivery list or an assertion failure in
PushStreamIterator.Changes
VirtualStorageCatalog::holdsReferenceto distinguish proxy reference ownership:e_PUSHholds a referenceTesting
Add unit coverage for:
Add integration coverage for:
The integration tests verify that the proxy remains alive and that every remaining App receives its messages.